-
Notifications
You must be signed in to change notification settings - Fork 217
Cancel health checks on new reconciliation request #1536
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Matheus Pimenta <[email protected]>
e34dbbc
to
dc6fc1d
Compare
WatchExternalArtifacts: true, | ||
WatchConfigsPredicate: predicate.Not(predicate.Funcs{}), | ||
WatchExternalArtifacts: true, | ||
CancelHealthCheckOnNewRevision: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How to have tests for both true
and false
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the lines you just deleted from the test:
reconciler.CancelHealthCheckOnNewRevision = true
t.Cleanup(func() { reconciler.CancelHealthCheckOnNewRevision = false })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the boolean moving from the reconciler to the SetupWithManager options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah right, the test framework we have in place does not cope with this, you can not rebuild the controller once it has started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a separate package like we did before?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe test only true since ideally we would not have this feature gate. If this works well we plan to make it opt-out later, and possibly even ignore it with a warning after even more time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah enabling it in tests should be Ok. The e2e test suite has the default gate state, so we cover both cases.
Part of: fluxcd/flux2#5584